-
Notifications
You must be signed in to change notification settings - Fork 68
Add if/else if/else expression to KQL parser
#1722
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add if/else if/else expression to KQL parser
#1722
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1722 +/- ##
==========================================
- Coverage 84.08% 83.94% -0.14%
==========================================
Files 469 468 -1
Lines 137651 135954 -1697
==========================================
- Hits 115746 114129 -1617
+ Misses 21371 21291 -80
Partials 534 534
🚀 New features to boost your workflow:
|
|
@albertlockett One FYI note about the usage of the if (severity_text == "ERROR") {
extend attributes["important"] = "very" // ignoring | extend attributes["triggers_alarm"] = "true"
} else if (severity_text == "WARN") {
extend attributes["important"] = "somewhat"
} else if (severity_text == "INFO") {
extend attributes["important"] = "rarely"
} else {
extend attributes["important"] = "no"
}could also be written as: logs
| extend attributes["important"] = case(severity_text == "ERROR", "very", severity_text == "WARN", "somewhat", severity_text == "INFO", "rarely", "no"That said, I think the actual important part of this PR here is actually executing multiple statements in a branch (to get the @CodeBlanch what other significant departures from pure KQL do we currently have? Off the top of my head, I recall our map accessing is a bit different. I wonder if we can have a superset of the grammar in a separate |
|
Hey @drewrelmas thanks for taking a look!
Yeah exactly. We'd like to add some additional conditional capabilities beyond just assignment, and routing would be part of that. I can imagine us wanting to do something like: logs |
if (severity_text == "ERROR") {
extend attributes["triggers_alarm"] = "true" | route_to "high_severity_out_port"
} else {
route_to "low_severity_out_port"
}
Thanks for sharing that! I didn't know about the multiple grammars, but I think a that's exactly what we'll need: a base grammar, and parsers that inherit the base, plus various extensions. I created an issue to track this work #1728 and I'll spend some time today investigating how to make this work. In terms of getting this PR merged, I'll leave it up to you whether you're comfortable merging as is, or whether it'd be better to implement a solution for #1728 first and rebase these changes on top of that. I foresee a few other divergences from standard KQL we'd like to support in the columnar query engine. These include operators |
lquerel
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
@drewrelmas wrt to:
I added a solution for this in #1734 Can you (and maybe @CodeBlanch) take a look and let me know what you think? The changes there will obviously conflict with the changes in this PR, but I'm open to having them merged in either order. |
closes #1743 Initial implementation of OPL parser. This has just enough functionality to support the filtering capabilities we've already implemented in the columnar query engine. I will integrate this with the test suite in that crate in a followup PR, then continue implementing the parser for various other pipeline stages such as attribute modification and if/else statement (continuing from work in #1722). I can fill in more documentation in future PRs as well.
|
Closing this for now. Will open a different PR moving this code to OPL Parser that was added in #1752 |
closes: #1777 relates to: #1667 This is followup from #1722 , where we decided to not implement this feature in KQL parser and instead have OPL implement it's own parser. Adds parsing support for an if/else if/else expression that gets parsed into the ConditionalDataExpression that was added in #1684 The syntax looks like this. ``` logs | if (severity_text == "ERROR") { extend attributes["important"] = "very" | extend attributes["triggers_alarm"] = "true" } else if (severity_text == "WARN") { extend attributes["important"] = "somewhat" } else if (severity_text == "INFO") { extend attributes["important"] = "rarely" } else { extend attributes["important"] = "no" } ``` `else if` and `else` are optional, so the following expressions are also supported: ``` logs | if (severity_text == "ERROR") { extend attributes["is_error"] = true } else { extend attributes["is_not_error"] = true } ``` ``` logs | if (severity_text == "ERROR") { extend attributes["triggers_alarm"] = "true" } ``` In a future PR, I'll go back and use `OplParser` in columnar query engine's [`pipeline/conditional.rs` unit tests](https://github.com/open-telemetry/otel-arrow/blob/cf43a4c0ddc170aa1b8ec243f8bb28f0bce591ba/rust/otap-dataflow/crates/query-engine/src/pipeline/conditional.rs#L227-L237). This can happen once [#1762](#1762) is also complete, as those tests use `project-rename`, so support for this must also be added to OPL Parser.
part of #1667
Adds parsing support for an
if/else if/elseexpression that gets parsed into theConditionalDataExpressionthat was added in #1684The syntax looks like this.
else ifandelseare optional, so the following expressions are also supported:If we're OK with the syntax and how this is implemented, I'll go back and clean up the tests that we're added in #1684 in a followup PR. Those tests are manually building the expression AST, but it might be easier to maintain them if we parsed the query.